Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove solidity packages and conf #286

Merged
merged 13 commits into from
May 20, 2024
Merged

chore: remove solidity packages and conf #286

merged 13 commits into from
May 20, 2024

Conversation

cedoor
Copy link
Member

@cedoor cedoor commented May 16, 2024

Description

This PR removes all code related to Solidity as it has been moved to another ZK-Kit repository: https://github.com/privacy-scaling-explorations/zk-kit.solidity.

It also optimizes the Github workflows and removes legacy JS packages (groth16 and rollup-plugin-rust).

Related Issue(s)

Closes #282

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have run yarn style without getting any errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@cedoor
Copy link
Member Author

cedoor commented May 16, 2024

@sripwoud @baumstern It would be nice to have your feedback for files related to turbo and workflows.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
@sripwoud
Copy link
Member

Not related to this PR, but to the new zk-kit.{circom,solidity,noir}:
Do we want to keep using turbo there? Not saying we should...I just don't see it used anymore.
I didn't test the scripts in these new repos to have a feeling for their speed and assess whether turbo would be beneficial or overkill.
I guess it is very likely overkill for the noir repo.
But maybe useful for circom that contains lot of subpackages already ...
and solidity?

cedoor and others added 5 commits May 17, 2024 10:03
Co-authored-by: sripwoud <[email protected]>
Co-authored-by: sripwoud <[email protected]>
Co-authored-by: sripwoud <[email protected]>
Co-authored-by: sripwoud <[email protected]>
Co-authored-by: sripwoud <[email protected]>
turbo.json Outdated
Comment on lines 26 to 34
"@zk-kit/imt#build",
"@zk-kit/lean-imt#build",
"@zk-kit/baby-jubjub#build",
"@zk-kit/eddsa-poseidon#build",
"@zk-kit/lazytower#build",
"@zk-kit/poseidon-cipher#build",
"@zk-kit/poseidon-proof#build",
"@zk-kit/smt#build",
"@zk-kit/utils#build"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sripwoud Is there a way to avoid listing all packages here? Isn't it possible to put 1 command somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't remember why I wrote this but there must have been a reason.

I remember it had to do with dealing with a root task and wanting to specify some workspace tasks as dependencies.
So I ended using this syntax
https://turbo.build/repo/docs/core-concepts/monorepos/task-dependencies#from-arbitrary-workspaces

now we use the root test task that needs everything to be build first.
So did you try simply?

   "//#test":{ "dependsOn": ["build"] ...}

@cedoor
Copy link
Member Author

cedoor commented May 17, 2024

I guess it is very likely overkill for the noir repo. But maybe useful for circom that contains lot of subpackages already ... and solidity?

I've not added it because I've not studied it yet. But if you think they could benefit from it please test them!

Copy link
Member

@sripwoud sripwoud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remaining comments are just some config tweaks improvements.
Nothing blocking merging imo.

utACK

turbo.json Outdated
Comment on lines 26 to 34
"@zk-kit/imt#build",
"@zk-kit/lean-imt#build",
"@zk-kit/baby-jubjub#build",
"@zk-kit/eddsa-poseidon#build",
"@zk-kit/lazytower#build",
"@zk-kit/poseidon-cipher#build",
"@zk-kit/poseidon-proof#build",
"@zk-kit/smt#build",
"@zk-kit/utils#build"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't remember why I wrote this but there must have been a reason.

I remember it had to do with dealing with a root task and wanting to specify some workspace tasks as dependencies.
So I ended using this syntax
https://turbo.build/repo/docs/core-concepts/monorepos/task-dependencies#from-arbitrary-workspaces

now we use the root test task that needs everything to be build first.
So did you try simply?

   "//#test":{ "dependsOn": ["build"] ...}

turbo.json Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@cedoor cedoor merged commit 02c1cae into main May 20, 2024
2 checks passed
@cedoor cedoor deleted the chore/remove-solidity branch May 20, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-organize ZK-Kit packages
2 participants